Repository navigation
ble: port the 5/MG CLIENT_HELLO suppression to iOS/macOS (#1635) - #1672
Merged
Merged
Conversation
Android has stopped the 5/MG reconnect loop since #1642; iOS never got it, so a 5/MG here still writes the hello every connect, loses the link ~3s later and reconnects forever - the exact behaviour v10.6.0 shipped on both platforms. The hello is what ends the link. An HCI capture settled why: the phone sends an SMP Pairing Request and the strap answers Pairing Not Supported (0x05), so the encrypted bond the hello waits behind can never arrive on a 5/MG. Suppressing it after the give-up latches trades an unreachable capability (puffin commands, history offload) for one that demonstrably works - continuous live HR, the "Live HR, not fully paired" state the app already models (#69). Ported, all mirroring the Kotlin twins: - shouldSendClientHello / giveUpSuppressesHello / countsAsBondRefusal / helloSuppressionPrefKey, in a new pure Strand/BLE/HelloSuppression.swift - helloSuppressedEpitaph / helloSuppressedHint / pausedHintHandshakeUnanswered on BondRefusalGiveUp - recordWhoop5BondRefusal, shared by both causes so the split cannot drift #1640 had to come with it. An unanswered hello produces no write error at all - the link just drops - so it never reached this platform's give-up, which only ever saw INSUFFICIENT_AUTHENTICATION. Without that half the suppression could never trigger. It is read at disconnect while clientHelloWriteAt and didBond are both still valid. Checked, because didBond stays permanently false once suppressed and CLAUDE.md names this the trap: iOS has NO bond watchdog and NO #982 never-bonded detector, so neither can undo it here. countsAsBondRefusal gates on family, so a 4.0 can never latch. An auth refusal cannot be double-counted or mislabelled as unanswered, because the write-error branch clears clientHelloWriteAt before the count. Only user-driven paths reach connect(), so the one-shot retry cannot leak into an automatic reconnect - automatic ones go through central.connect directly. SettingsView had a comment left for whoever did this port: its status line falls through to "Finishing the secure pairing handshake" and would describe a handshake no longer being attempted. It now carries the same "streaming but not fully paired" arm as SettingsLogic.strapStatusLine, translated into all ten locales, placed after the hint so the actionable text still wins. NOT ported: the #1669 opt-in override. It is experimental, default-off, and on hardware it produced 57 reconnect cycles in an hour before #1670 bounded it - not a thing to add to a second platform before the first one has field evidence. Verification: the three hint strings are proven byte-identical to the Kotlin source by extracting both and comparing, not by eye. 13 tests over the pure rules, incl. the family gate across every DeviceFamily case and the latch's case-insensitivity. The pure file also compiled standalone with swiftc and every rule exercised, since the app target has no default CI. doc_comment_lint clean; i18n audit 0 gaps in the gated locales. NOT hardware-validated: I have no 5/MG on iOS to test against, and BLE behaviour cannot be CI-tested. The falsifiable prediction is the same one Android's fix carried - after the latch, the loop stops and live HR streams continuously. If a link still drops on a cycle with no CLIENT_HELLO line, the port is wrong.
Re-review found a fourth didBond reader I had missed. CLAUDE.md warns that didBond is load-bearing well beyond the handshake; I audited the bond watchdog, the #982 detector and the give-up, and stopped there. The keep-alive timer reads it too. keepAliveFire gated on didBond, which a suppressed 5/MG never reaches. That switched the whole timer off for exactly the link the suppression keeps up for hours, taking the liveness watchdog with it - so a stalled stream would have had nothing left to bounce it, and the "stable live HR" this port promises would have quietly depended on a stream nothing was watching. The Android twin gates on `bonded`, the live-HR shortcut, and so keeps reaching its watchdog. Worse, the first attempt at this was dead code: BOTH existing startKeepAlive() call sites are post-bond, so on a suppressed link the timer never starts at all and a corrected gate changes nothing. It now starts at the suppression point too. Safe before HR arrives, because the gate returns false until the stream marks the link established (#8). Deliberately narrower than Android's gate: only a 5/MG is admitted unbonded, and only as far as the watchdog - a second didBond guard closes the door before anything puffin-framed, which an unbonded link cannot land anyway. A 4.0 sitting unbonded is a real fault, not a suppressed link, and must not be kept alive. Verification: the gate is extracted as pure keepAliveMayRun so it is pinned without a radio - 4 tests incl. every DeviceFamily case either side, and the whole pure file re-compiled standalone with swiftc and every rule exercised. doc_comment_lint clean. Still not hardware-validated.
forgetDevice states the invariant plainly - releasing a strap fully resets the give-up so no state outlives the strap it belonged to and wedges a later re-add. The suppression latch is exactly that kind of state, and unlike the give-up it is PERSISTED, so it survived the removal: a re-added strap would have had its handshake silently suppressed by a verdict reached before the user removed it, and a stale UserDefaults key was left behind for a strap that is gone. The identifier is captured before the teardown nils `peripheral`, because a nil argument means "the current strap" and that is the only place it can still be read. Re-latching costs the same five refusals it always did, so a strap that genuinely cannot bond is no worse off. That completes the latch's lifecycle: set on the give-up trip, read at the hello gate, cleared on a genuine bond, and now cleared on release. doc_comment_lint clean. Still not hardware-validated.
A parity audit of the port against the Kotlin original turned up divergences in BOTH directions. Android was missing the release fix. releaseStrap() resets the give-up under the stated invariant that no state outlives the strap it belonged to, but the hello-suppression latch is persisted and survived it - so a re-added strap kept a verdict reached before the user removed it, and a stale pref was left behind. The Swift twin landed a commit ago; this is the Kotlin half, and the parity contract wants both in the one PR. The address is captured before the clearing nulls lastDevice, which lastDeviceAddress reads through. iOS started the keep-alive in the wrong place. I put it at the suppression point; Android starts it on the live-HR bonded transition, which also covers an unbonded 5/MG that has NOT latched yet - during the give-up's first five cycles iOS would have had no liveness watchdog where Android does. Moved to the transition, so there is now one trigger and it is the same one on both platforms. Also checked, no change needed: Android's keep-alive gate looks broader (`s.bonded`, any family) but its live-HR shortcut is itself family-gated to non-WHOOP4, so a 4.0 can only reach it through a genuine bond - equivalent to the narrower Swift gate, which is merely more explicit about it. Verification: Android 4552 tests green, compile clean. app-build was green on the previous head (Build Strand, Test Strand, Build NOOPiOS) and is re-dispatched for these. Still not hardware-validated on either platform.
ryanbr
added a commit
that referenced
this pull request
Aug 27, 2026
…OS) (#1673) The gap #1672 left open. iOS issued the DIS identity read only inside the post-bond handshake, so a 5/MG that never bonds never read DIS at all - and since the #1635 suppression landed, "never bonded" is a PERMANENT state on these straps rather than a transient one. The Devices screen showed a WHOOP 4.0 with its firmware beside a 5/MG with none, and MG-only capabilities stayed locked because the strap never got to attest what it is. Android has had this since #1647/#1649/#1653. Ported: shouldReadDisUnbonded, disRefusedPrefKey, disReadFailureLine and shouldPublishDisFirmware as a pure Strand/BLE/DisUnbondedRead.swift twinning DisUnbondedRead.kt; the unbonded attempt itself, scheduled from the suppressed path and nowhere else, because that is the only 5/MG state known to be stable; and the four DIS characteristics this platform never read - firmware (0x2A26), manufacturer, model number, software revision. Firmware is the point of it. DIS 0x2A26 is the ONLY source an unbonded strap has, since the puffin report needs the bond that never happens - and #1634 records that the 5/MG hello frequently decodes no firmware even when it DOES bond, so this fills a real void rather than racing a value that is coming. Published as a FALLBACK, never an override. Self-limiting, deliberately. A read is the same class of operation as the write that provokes the teardown, so this could cost the stable link the suppression just bought: once per link, and never again on a strap that has refused once. A refusal is a RESULT - it settles #490, which no capture has ever done - so it is reported with its status rather than swallowed by the generic notify-failure line. Five review passes found five defects, and the last two are the ones worth reading: - the ECG PROBE was blocked only by accident. Its gate checks opt-in, attested MG and connected - but not the bond. On a suppressed strap the MG check blocked it incidentally, because the variant could only resolve from a post-bond read. Reading DIS unbonded removes that accident, and the gate would then have let the probe write puffin frames to fd4b0002 - the very characteristic whose write tears the link down, spending the stable link the suppression exists to buy. send() is no backstop: it guards connected, not bonded. The gate now requires the encrypted bond, as the ECG GATE-WRITE path always did (#269). - the per-device firmware key had readers and NO writer. I first claimed iOS had no FirmwareAttribution twin; that was wrong, and wrong because I grepped Kotlin's names against Swift. It exists, has tests, and is read by DevicesView and the debug export - but the decoded path cannot key on identity, which its own comment concedes, so perDevice always resolved nil and both readers fell back to a global value that is wrong the moment a second strap exists. The DIS site has the peripheral, so it is the one place that can attribute a firmware honestly. That fills a hole predating this change. Also from the passes: the delayed read now checks `connected`, because `peripheral` outlives a disconnect on this platform and the line would otherwise claim an attempt that never happened; three comments still describing the read as post-bond-only were corrected; and the delay is pinned to the twin's actual number (1500 x 2 = 3.0s, not the 4.0 I first guessed). Deliberate divergences, all documented in code: the extras are issued together rather than chained, because CoreBluetooth already serialises reads; the refusal latches only for an UNBONDED attempt, since a transient failure on a bonded link would poison a permanent latch; there is no explicit-bond-deferred call site, that experiment being Android-only; and the post-bond path stays silent about a missing serial, because it is re-entered from the keep-alive tick and would log every cycle. Verification: disReadFailureLine and both firmware log lines proven byte-identical to the Kotlin source by extracting and comparing - which caught a hyphen where Kotlin has an em-dash. All six DIS UUIDs verified against the twin. 14 tests over the pure rules incl. every arm of the attempt gate, the fallback-not-override rule, and that the DIS latch key cannot collide with the hello-suppression one; the pure file also compiled standalone with swiftc and every rule exercised. doc_comment_lint and the i18n gate clean. NOT hardware-validated, and this one is genuinely unproven rather than merely untested: whether a 5/MG answers a standard read on an unencrypted link is the open question #490 asks, and nothing here can settle it without a strap. Either outcome is informative, and a refusal costs one read per strap, once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Android has stopped the 5/MG reconnect loop since #1642. iOS never got it, so a 5/MG here still writes the CLIENT_HELLO every connect, loses the link ~3s later and reconnects forever — the behaviour v10.6.0 shipped on both platforms.
The hello is what ends the link. An HCI capture settled why: the phone sends an SMP Pairing Request and the strap answers
Pairing Not Supported(0x05), so the encrypted bond the hello waits behind can never arrive on a 5/MG. Suppressing it after the give-up latches trades an unreachable capability (puffin commands, history offload) for one that demonstrably works — continuous live HR, the "Live HR, not fully paired" state the app already models (#69).What was ported
shouldSendClientHello,giveUpSuppressesHello,countsAsBondRefusal,helloSuppressionPrefKeyStrand/BLE/HelloSuppression.swifthelloSuppressedEpitaph,helloSuppressedHint,pausedHintHandshakeUnansweredBondRefusalGiveUprecordWhoop5BondRefusal, shared by both causes#1640 had to come with it. An unanswered hello produces no write error at all — the link just drops — so it never reached this platform's give-up, which only ever saw
INSUFFICIENT_AUTHENTICATION. Without that half the suppression could never trigger. It's read at disconnect whileclientHelloWriteAtanddidBondare both still valid.The didBond audit
didBondstays permanently false once suppressed, and CLAUDE.md names this the trap that undid the Android change twice. Checked here:countsAsBondRefusalgates on family — a 4.0 can never latch.clientHelloWriteAtbefore the count.connect(), so the one-shot retry can't leak into an automatic reconnect — those go throughcentral.connectdirectly.SourceCoordinator.startWhoopfires only on user-driven device transitions and is a no-op for the single-WHOOP default.The status line
SettingsViewhad a comment left for whoever did this port: it falls through to "Finishing the secure pairing handshake…" and would describe a handshake no longer being attempted. It now carries the same "streaming but not fully paired" arm asSettingsLogic.strapStatusLine, translated into all ten locales, placed after the hint so the actionable text still wins.Not ported
The #1669 opt-in override. It's experimental, default-off, and on hardware produced 57 reconnect cycles in an hour before #1670 bounded it — not a thing to add to a second platform before the first has field evidence.
Verification
The three hint strings are proven byte-identical to the Kotlin source by extracting both and comparing, not by eye. 13 tests over the pure rules, including the family gate across every
DeviceFamilycase and the latch's case-insensitivity. The pure file was also compiled standalone withswiftcand every rule exercised, since the app target has no default CI.doc_comment_lintclean; i18n audit 0 gaps in the gated locales. app-build dispatched.NOT hardware-validated. I have no 5/MG on iOS to test against, and BLE behaviour cannot be CI-tested. The falsifiable prediction is the one Android's change carried: after the latch, the loop stops and live HR streams continuously. If a link still drops on a cycle with no CLIENT_HELLO line, the port is wrong.